fix(iac): rename DriftConfigDetector.DetectDriftWithApplied → DetectDriftWithSpecs (align with DO plugin v0.10.5)#571
Merged
Conversation
…DetectDriftWithSpecs
Aligns workflow's interface with the signature shipped in workflow-plugin-
digitalocean v0.10.5. The DO plugin chose map[string]ResourceSpec (typed
wrapper with Name/Type/Config fields) over map[string]map[string]any; the
typed form is cleaner and avoids callers having to reconstruct name/type
from keys.
Wire protocol change: remoteIaCProvider.DetectDriftWithSpecs now invokes
IaCProvider.DetectDrift with a "specs" arg map rather than a separate
IaCProvider.DetectDriftWithApplied RPC method. This matches the DO plugin
v0.10.5 dispatch (specs injection is gated inside invokeProviderDetectDrift
on arg presence). Plugins predating DriftConfigDetector support still handle
IaCProvider.DetectDrift; they ignore the unknown "specs" key and return
existence-only results.
buildAppliedSpecMap return type changed to map[string]interfaces.ResourceSpec;
each entry is wrapped as ResourceSpec{Name, Type, Config}. All callers and
tests updated. ADR 0007 extended with addendum documenting the rename and
wire protocol change.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns the IaC drift-detection optional interface and wfctl RPC dispatch with workflow-plugin-digitalocean v0.10.5 by switching from an “applied config map” to a typed ResourceSpec map and routing spec injection through the existing IaCProvider.DetectDrift RPC with a "specs" argument.
Changes:
- Renames
DriftConfigDetector.DetectDriftWithApplied→DetectDriftWithSpecsand updates the argument type tomap[string]ResourceSpec. - Updates wfctl drift/refresh callers and the
buildAppliedSpecMaphelper to constructResourceSpec{Name, Type, Config}entries. - Updates remote IaC provider wire protocol to call
IaCProvider.DetectDriftwith"refs"+"specs"args, plus corresponding tests/docs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| interfaces/iac_provider.go | Renames optional drift interface method and changes the parameter type to ResourceSpec. |
| interfaces/iac_provider_test.go | Updates optional-interface compile-time assertion test to new method signature. |
| decisions/0007-iac-driftconfigdetector-optional-interface.md | Documents the rename and explains the updated wire-protocol approach. |
| cmd/wfctl/infra_status_drift.go | Updates drift path to call DetectDriftWithSpecs when supported. |
| cmd/wfctl/infra_apply_refresh.go | Updates refresh drift path to call DetectDriftWithSpecs when supported. |
| cmd/wfctl/infra_drift_applied.go | Changes helper to return map[string]ResourceSpec and wraps applied config accordingly. |
| cmd/wfctl/infra_drift_applied_test.go | Updates tests for buildAppliedSpecMap to validate ResourceSpec output. |
| cmd/wfctl/deploy_providers.go | Renames remote wrapper method and sends "specs" via IaCProvider.DetectDrift. |
| cmd/wfctl/deploy_providers_remote_iac_test.go | Updates remote IaC tests for new method name and wire-method expectation. |
| cmd/wfctl/deploy_providers_remote_iac_compat_test.go | Updates compat test to pin “specs via DetectDrift” protocol expectation. |
…hSpecs refactor The isMethodNotFound helper was only referenced inside the old DetectDriftWithApplied wrapper. DetectDriftWithSpecs now dispatches via IaCProvider.DetectDrift (always available), so no method-not-found fallback is needed at the wire level. Remove the unused function to satisfy golangci-lint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three real findings from Copilot review round 1: 1. buildAppliedSpecMap: prefer st.Type (canonical from ResourceState) over ref.Type when building ResourceSpec. Ref may be a lightweight lookup without full type info; state is authoritative. Falls back to ref.Type when st.Type is empty for backwards compat with state records that predate Type recording. 2. Test assertion: verify InvokeService is called with "specs" key present and legacy "applied" key absent — pins the wire contract. 3. New test TestBuildAppliedSpecMap_PrefersStateTypeOverRefType covers the st.Type → spec.Type path. One ghost flag resolved without change: Copilot flagged ctx parameter in DetectDriftWithSpecs as "will fail compilation" — Go does not fail on unused function parameters (only unused local variables). Build confirms this is safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Matches the _ context.Context pattern used by all other remoteIaCProvider methods that call InvokeService (which does not accept a context). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…R 0007 signatures - DetectDriftWithSpecs now mirrors RepairDirtyMigration: type-asserts invoker to remoteServiceContextInvoker and calls InvokeServiceContext(ctx, ...) when available, falling back to InvokeService with ctx.Err() guard. - ADR 0007 Decision code blocks updated from DetectDriftWithApplied to DetectDriftWithSpecs and map[string]map[string]any to map[string]ResourceSpec to match the v0.22.3 canonical interface; labels pre-v0.22.3 context as historical. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and wire protocol accuracy - interfaces/iac_provider.go: change 'desired-spec map' to 'applied-config map sourced from state' in the DetectDriftWithSpecs doc comment — the specs come from ResourceState.AppliedConfig (applied/user-supplied config), not a desired/intended future state. - decisions/0007: qualify wire protocol claim about unknown-key handling — only true for LEGACY_STRUCT/PROTO_WITH_LEGACY_STRUCT plugins; STRICT_PROTO plugins must include an optional specs field in their DetectDrift proto message. - decisions/0010: rename DetectDriftWithApplied → DetectDriftWithSpecs in all references to match the v0.22.3 interface rename. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workflow-plugin-digitalocean shipped v0.10.5 with its own implementation of the DriftConfigDetector interface using a different signature than our locked plan specified:
DetectDriftWithApplied(ctx, refs, applied map[string]map[string]any)DetectDriftWithSpecs(ctx, refs, specs map[string]interfaces.ResourceSpec)The DO plugin's typing is cleaner —
ResourceSpeccarriesName,Type, andConfigin one typed value. Per the autonomous mandate ("build the right way; refactor where needed"), we adopt the DO plugin's signature as canonical.Changes
interfaces/iac_provider.go—DriftConfigDetector.DetectDriftWithAppliedrenamed toDetectDriftWithSpecs; parameter type changed frommap[string]map[string]anytomap[string]ResourceSpeccmd/wfctl/infra_drift_applied.go—buildAppliedSpecMapreturn type changed tomap[string]interfaces.ResourceSpec; each entry wrapped asResourceSpec{Name, Type, Config}cmd/wfctl/infra_apply_refresh.goandinfra_status_drift.go— call sites updated toDetectDriftWithSpecscmd/wfctl/deploy_providers.go—remoteIaCProvider.DetectDriftWithAppliedrenamed toDetectDriftWithSpecs; wire protocol updated: now callsIaCProvider.DetectDriftwithspecsarg (not a separate RPC method name). This matches the DO plugin v0.10.5 dispatch — it checks for"specs"in args insideinvokeProviderDetectDriftdecisions/0007-iac-driftconfigdetector-optional-interface.md— addendum documenting the rename and wire protocol rationaleWire protocol note
The original plan called
IaCProvider.DetectDriftWithAppliedas a new RPC method. The DO plugin v0.10.5 routes spec-injection throughIaCProvider.DetectDriftby checking for a"specs"key in the args map — no new RPC case needed. The new wfctl implementation sendsIaCProvider.DetectDriftwithrefs+specsargs. This is more robust: plugins that predate DriftConfigDetector support still handleIaCProvider.DetectDriftand return existence-only results (ignoring unknown args).Test plan
go test ./interfaces/...— DriftConfigDetector interface compile-time assertion passes with new method namego test ./cmd/wfctl/ -run TestBuildAppliedSpecMap— ResourceSpec return type + nil-guard tests passgo test ./cmd/wfctl/ -run TestRemoteIaC_DetectDrift— wire protocol test: confirmsIaCProvider.DetectDriftmethod name is used, notDetectDriftWithAppliedgo test ./cmd/wfctl/ -run TestRemoteIaC_DriftConfigDetector— compat test passesgrep -r DetectDriftWithApplied .— returns empty (no stale references)🤖 Generated with Claude Code